London | 26-ITP-January | Damian Dunkley | Sprint 1 | coursework/sprint-1#949
London | 26-ITP-January | Damian Dunkley | Sprint 1 | coursework/sprint-1#949DamianDL wants to merge 21 commits intoCodeYourFuture:mainfrom
Conversation
…nd explanation included as text
…ation/answers included as text
DamianDL
added
📅 Sprint 1
MorganDavid
added
Needs Review
DamianDL
added
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
DamianDL
added
the
Needs Review
jennethydyrova
added
Review in progress
jennethydyrova
left a comment
jennethydyrova
left a comment
There was a problem hiding this comment.
Hi @DamianDL. Solid work! I left couple of comments as a stretch to explore how you could improve your code. Two comments in Sprint-1/3-mandatory-interpret/2-time-format.js file require changes.
Sprint-1/1-key-exercises/3-paths.js
Outdated
| const dir = ; | ||
| const ext = ; | ||
| const dir = filePath.slice(0, lastSlashIndex); | ||
| const ext = filePath.slice(lastSlashIndex + 1).split(".").pop(); |
There was a problem hiding this comment.
You could do this slightly simpler. Do you already have a variable that does filePath.slice(lastSlashIndex + 1)?
Sprint-1/2-mandatory-errors/0.js
Outdated
| @@ -1,2 +1,4 @@ | |||
| This is just an instruction for the first activity - but it is just for human consumption | |||
| We don't want the computer to run these 2 lines - how can we solve this problem? No newline at end of file | |||
| //This is just an instruction for the first activity - but it is just for human consumption | |||
There was a problem hiding this comment.
I am a little bit nitpicking here but we leave a space between // and your comment. That would be // Foo
Sprint-1/2-mandatory-errors/0.js
Outdated
| This is just an instruction for the first activity - but it is just for human consumption | ||
| We don't want the computer to run these 2 lines - how can we solve this problem? No newline at end of file | ||
| //This is just an instruction for the first activity - but it is just for human consumption | ||
| //We don't want the computer to run these 2 lines - how can we solve this problem? |
There was a problem hiding this comment.
Another, more conventional way to write multi-line comments is shown here: https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Syntax/Comments#examples.
That said, this approach works as well.
|
|
||
| // There are 6 variable declarations in this program. They are: movieLength, remainingSeconds, totalMinutes, remainingMinutes, totalHours, and result. | ||
| // b) How many function calls are there? | ||
| // There are 2 function calls in this program. They are: the template literal function call when assigning a value to the result variable (line 9) and the console.log function call on line 10. |
There was a problem hiding this comment.
Line 9 isn’t a function call, it’s just a template literal creating a string using backticks and ${} interpolation. No function is being executed there because there are no () parentheses.
|
|
||
| // e) What do you think the variable result represents? Can you think of a better name for this variable? | ||
|
|
||
| // The variable result is the length of the movie in hours, minutes, and seconds. A better name for this variable could be movieDurationhms, as it more clearly indicates that it is the duration of the movie in hours, minutes, and seconds. |
There was a problem hiding this comment.
Your suggestion for variable name movieDurationhms is good but to follow the camelCase rule of naming variables, it would need to be movieDurationHms. I would suggest to simplify it to movieDuration as Hms is a bit cryptic and not very clear for a code reader.
jennethydyrova
added
Reviewed
…paths.js and to 2-mandatory-errors/0.js
DamianDL
added
Needs Review
jennethydyrova
added
Reviewed
|
Good work! |
|
Hi @jennethydyrova. Thanks for reviewing my PR again. I am unclear what still needs correcting/updating/changing for this to be completed? Please can you help me understand what is outstanding? Thanks Damian |
jennethydyrova
added
Complete
|
Hi @DamianDL! Nope, I forgot to change the label. It should be all good now |
|
Thanks @jennethydyrova, great news. |
3 participants
Learners, PR Template
Self checklist
Changelist
I have completed the three mandatory exercises in sprint -1; the 4 exercises in 1-key-exercises, explained and corrected the 4 errors in 2-mandatory-errors and explained how the code works in 3-mandatory-interpret. I have not yet completed the 4-stretch-explore exercises, but understand that this is not critical for this PR.
I have used the reference documentation;
https://www.google.com/search?q=get+first+character+of+string+mdn
https://www.google.com/search?q=slice+mdn
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Arithmetic_Operators
Questions